Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/absences #12

Merged
merged 36 commits into from
Mar 9, 2021
Merged

Feature/absences #12

merged 36 commits into from
Mar 9, 2021

Conversation

philipflohr
Copy link
Contributor

@philipflohr philipflohr commented Nov 30, 2020

This adds absence handling and raw api tests for absences and mock api tests for absence creation, deletion and absence retrieval

@codecov-io
Copy link

codecov-io commented Nov 30, 2020

Codecov Report

Merging #12 (df95c36) into master (2d61dd3) will decrease coverage by 0.20%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   84.34%   84.13%   -0.21%     
==========================================
  Files           6        6              
  Lines         728      769      +41     
==========================================
+ Hits          614      647      +33     
- Misses        114      122       +8     
Impacted Files Coverage Δ
src/personio_py/client.py 70.94% <58.13%> (-7.38%) ⬇️
src/personio_py/models.py 88.17% <76.92%> (+3.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d61dd3...df95c36. Read the comment docs.

@philipflohr philipflohr changed the title [WIP] Feature/absences Feature/absences Dec 2, 2020
@philipflohr
Copy link
Contributor Author

Test are added. @klamann can you have a look at the changes?

src/personio_py/client.py Outdated Show resolved Hide resolved
@klamann
Copy link
Contributor

klamann commented Dec 3, 2020

there's a lot of changes in here, I'll need some time to look into it

@klamann
Copy link
Contributor

klamann commented Dec 4, 2020

Hey @philipflohr, I've set up a matrix chat for this project at #personio-py:matrix.org. Would you like to meet up there and have a chat? Because you're doing quite a bit of work on these PRs and I don't have that much time to work on this project right now, I think it would be helpful if we had a more efficient way of communicating.

src/personio_py/models.py Outdated Show resolved Hide resolved
src/personio_py/models.py Outdated Show resolved Hide resolved
@philipflohr
Copy link
Contributor Author

Hi @klamann
it has been quite a while since our talk...
So far done:

  1. remote id queries are disabled for delete operations and enabled for get operations.
  2. 6fb32a4 fixes an issue (caused by changed api behavior?) where some objects where retrieved twice if paginated requests are used
  3. Updates for mock data to represent the changes in 6fb32a4
  4. Mock tests restructured to be more readable

Currently still missing: mocking functions and mocking data restructuring. Just to make sure I remember stuff correctly: You want all mocking data to be in a single file and all the mocking functions in one separate file?

@philipflohr
Copy link
Contributor Author

@klamann

Currently still missing: mocking functions and mocking data restructuring. Just to make sure I remember stuff correctly: You want all mocking data to be in a single file and all the mocking functions in one separate file?

was this correct?

@klamann
Copy link
Contributor

klamann commented Jan 21, 2021

Hey, sorry for the delay. Yes, the mock data should go in one file, but you can split the tests that use the mock data in separate files if you prefer.

@philipflohr
Copy link
Contributor Author

@klamann done. From my point of view this PR is now ready to be merged

@klamann
Copy link
Contributor

klamann commented Jan 22, 2021

ok thanks! I might be able to review this later today

Copy link
Contributor

@klamann klamann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work! I really like the detailed mock tests that you included.

I left a few comments, mostly about code style, but there are also 1-2 potential bugs in there I would like you to have a look at.

src/personio_py/client.py Show resolved Hide resolved
src/personio_py/client.py Outdated Show resolved Hide resolved
src/personio_py/client.py Outdated Show resolved Hide resolved
src/personio_py/client.py Outdated Show resolved Hide resolved
src/personio_py/client.py Outdated Show resolved Hide resolved
src/personio_py/client.py Outdated Show resolved Hide resolved
src/personio_py/models.py Outdated Show resolved Hide resolved
src/personio_py/models.py Outdated Show resolved Hide resolved
src/personio_py/models.py Show resolved Hide resolved
@klamann
Copy link
Contributor

klamann commented Jan 22, 2021

oh, one more thing: please update the README (section API Functions) and add a CHANGELOG entry for this PR

@philipflohr
Copy link
Contributor Author

@klamann if I'm not missing something all your remarks were addressed.

@klamann
Copy link
Contributor

klamann commented Mar 9, 2021

hey @philipflohr, sorry for letting you wait so long, I didn't notice that you were done with the changes. This looks fine for me, I'll merge it.

Should we create a new release, now that absences are implemented, or would you like to wait for #13?

@klamann klamann merged commit 39de44c into at-gmbh:master Mar 9, 2021
@philipflohr
Copy link
Contributor Author

Thanks for merging.
I'd suggest to create a new release as I don't know when I can finish #13 .

@klamann
Copy link
Contributor

klamann commented Mar 10, 2021

ok, done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants